feat: Add SNMP Monitor #4717

mattv8 · 2024-04-27T01:18:36Z

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Description

This PR introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol). The following changes have been made:

Added new fields for SNMP configuration, including community string, OID (Object Identifier), SNMP version, control value, and condition.
Updates the database schema to include SNMP-related columns.
Implemented SNMP check logic to determine device status based on SNMP values and configured conditions.
Utilizes net-snmp.

Resolves #1675

Type of change

Please delete any options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).

net-snmp over snmp-native is: -more robust -more popular -better documented -supports v3

Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.

@CommanderStorm

Thanks for pointing it out @CommanderStorm!

DB must allow nulls otherwise this will break other monitors.

knex requires down function

Updates appropriate values async when editing the SNMP monitor

mattv8 · 2024-04-30T21:51:04Z

@CommanderStorm I'm ready for review. It is not passing ES Lint, although when I run npm run lint:prod --fix there are some changes to server/model/monitor.js that are unrelated to this PR. I'm unsure as how to proceed on the linting side of things.

CommanderStorm

Please have a look at https://github.com/louislam/uptime-kuma/pull/4717/files or https://github.com/louislam/uptime-kuma/actions/runs/8902151438/job/24447507847?pr=4717 and fix the linting mistakes.
I don't know why your local environment is producing different linting results.

I am certain that a review at this time will miss too many things and will thus require another review cycle. Nevertheless, I have attached a few comments.

src/pages/.EditMonitor.vue.swp

src/pages/EditMonitor.vue

server/monitor-types/snmp.js

src/pages/EditMonitor.vue

db/knex_migrations/2024-04-26-0000-snmp-monitor.js

server/monitor-types/snmp.js

CommanderStorm · 2024-04-30T22:29:37Z

server/monitor-types/snmp.js

+                switch (monitor.snmpCondition) {
+                    case '>':
+                        heartbeat.status = numericValue > monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '>=':
+                        heartbeat.status = numericValue >= monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '<':
+                        heartbeat.status = numericValue < monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '<=':
+                        heartbeat.status = numericValue <= monitor.snmpControlValue ? UP : DOWN;
+                        break;
+                    case '==':
+                        if (!isNaN(value) && !isNaN(monitor.snmpControlValue)) {
+                            // Both values are numeric, parse them as numbers
+                            heartbeat.status = parseFloat(value) === parseFloat(monitor.snmpControlValue) ? UP : DOWN;
+                        } else {
+                            // At least one of the values is not numeric, compare them as strings
+                            heartbeat.status = value.toString() === monitor.snmpControlValue.toString() ? UP : DOWN;
+                        }
+                        break;
+                    case 'contains':
+                        heartbeat.status = stringValue.includes(monitor.snmpControlValue) ? UP : DOWN;
+                        break;
+                    default:
+                        heartbeat.status = DOWN;
+                        heartbeat.msg = `Invalid condition: ${monitor.snmpCondition}`;
+                        break;
+                }


About this part @chakflying has commented in #1675 (comment)

Sounds pretty good, but just in case you didn't know, you should take a look at how the json-query monitor works.

Ideally in the long term, we would want all value comparisons to work with the jsonata syntax, and reuse the database fields for better compatibility (see #3919). I don't think it's worth implementing custom value comparison functionality just for this monitor.

=> This needs to be compatible with #4617 and #3919

@chakflying what do you think is the best course forward?

drop this part until Proposal: Make Keyword & Json-Query generic across monitor types #3919 is done or

wait for feat: extend monitor JSON data evaluation operators #4617, then make this compatible and then do Proposal: Make Keyword & Json-Query generic across monitor types #3919? or

do the work here, wait for feat: extend monitor JSON data evaluation operators #4617 to be compatible and then do Proposal: Make Keyword & Json-Query generic across monitor types #3919?

@chakflying & @CommanderStorm I propose waiting until after #4617 is committed and I will re-factor & maintain the SNMP monitor after the fact.

server/monitor-types/snmp.js

Co-authored-by: Frank Elsinga <frank@elsinga.de>

https://github.com/louislam/uptime-kuma/pull/4717/files/b4bd003626986be1faab7142710f9443273d89fc#r1585590243

Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>

CommanderStorm · 2024-05-23T11:36:09Z

@chakflying (sorry to ping you, hope this is fine ^^)
What do you think about the approach used in this PR?
Is this acceptable or should we "force" the users to use json-query expressions publicly (with a longer helptext how the expressuions above work) as well?

chakflying · 2024-05-24T11:33:53Z

The idea behind my suggestion of using json-query for value comparisons, is that it can provide the maximum freedom for users to process and transform the input data before comparison.
If all we are using the library for is running a few preset queries, we are not providing that freedom to the user. Then we may as well have not used the library.
However, I also see that there is benefit in being able to set a custom condition/comparison operator other than ==, since it can simplify the expression in complicated cases. (Basically the idea in feat: extend monitor JSON data evaluation operators #4617)

My ideal implementation would be like this:

We reuse existing database columns json_path and expected_value, instead of creating new one snmpControlValue
We name the new column for the comparison operator json_path_operator instead of the current snmp_condition (as it was in feat: extend monitor JSON data evaluation operators #4617)
Optionally, we can default json_path to $.value for people who do not require any transformation.
When the monitor runs its check,
- we obtain the value from SNMP as it's currently done: const value = varbinds[0].value;
- we evaluate this value according to user's inputted json_path:
```
   let expression = jsonata(this.jsonPath);
   let result = await expression.evaluate(value);
```
- We compare result with expected_value using the user's chosen operator (==, >=, etc)
- This comparison produces a true/false result which will determine the monitor status

This implementation would be maximally compatible with the existing json-query monitor, and have the benefit that the user has much more freedom, e.g. can quickly check if the SNMP value is odd by doing $.value % 2 != 0 (I think).

I really appreciate the work that you have put into this feature, it looks great already. I don't want this to be like "great, but here's more work for you". It's difficult to reach an ideal solution when we all have different ideas of how it would look like. I'm also not in a good position to comment since I don't have an SNMP device to test. Feel free to discuss if needed.

chakflying · 2024-05-24T11:40:40Z

BTW, if we are reusing radiusPassword for the storing the community string, then I guess a new database column for that is also not needed anymore?

mattv8 · 2024-05-28T00:47:29Z

Hey @chakflying I appreciate your thoughts on all this! Currently on mobile and on vacation, so forgive this for being brief. I'll jump back into this in a week or so when I'm back in town and solid internet.

Yes, what you're describing I feel is absolutely workable. I think we need to walk the line between absolute freedom of an open text input for json-queries and the simplicity of the comparison drop down. Let me work through this and see what I can come up with!

SNMP Initial Commits

d92003e

This commit introduces a new SNMP monitor feature to the application, allowing users to monitor devices using SNMP (Simple Network Management Protocol).

mattv8 mentioned this pull request Apr 27, 2024

SNMP Value Retrieval #1675

Open

1 task

mattv8 changed the title ~~feat: Add SNMP Monitor~~ feat: Add SNMP Monitor (WIP) Apr 27, 2024

mattv8 added 3 commits April 29, 2024 13:58

Use net-snmp instead of snmp-native

a3cdd69

net-snmp over snmp-native is: -more robust -more popular -better documented -supports v3

Updated a comment

ff5890a

Further SNMP monitor development

4a882be

Further testing of SNMP feat, however I'm running into the issue `Error in SNMP check: RequestTimedOutError: Request timed out` when the check function is called. I am unsure as to why since my local SNMP script works great with very similar code.

This comment has been minimized.

Sign in to view

Wrong variable used

99dc4cf

Thanks for pointing it out @CommanderStorm!

This comment was marked as resolved.

Sign in to view

mattv8 added 6 commits April 30, 2024 15:10

Update db migration: allow nulls

138075a

DB must allow nulls otherwise this will break other monitors.

Update db migration: down function

9c8024c

knex requires down function

Update bean model backend

9d28fcf

Updates appropriate values async when editing the SNMP monitor

Frontend input validation

4593afb

Minor frontend styling

9848ce4

Finalized SNMP monitor

704ffd3

mattv8 changed the title ~~feat: Add SNMP Monitor (WIP)~~ feat: Add SNMP Monitor Apr 30, 2024

mattv8 marked this pull request as ready for review April 30, 2024 21:46

Merge branch 'master' into snmp-monitor

b4bd003

CommanderStorm requested changes Apr 30, 2024

View reviewed changes

mattv8 and others added 6 commits April 30, 2024 17:54

Apply suggestions from code review

ba47aca

Co-authored-by: Frank Elsinga <frank@elsinga.de>

ES Lint Compliant

7459654

Corrected down function

e944492

https://github.com/louislam/uptime-kuma/pull/4717/files/b4bd003626986be1faab7142710f9443273d89fc#r1585590243

ES Lint Compliant

97a9094

Remove supurfluous log.debug

9ba0f68

Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>

Delete .EditMonitor.vue.swp

ba84f01

Co-Authored-By: Frank Elsinga <frank.elsinga@tum.de>

github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 19, 2024

github-actions bot added needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again and removed needs:resolve-merge-conflict A merge-conflict needs to be addressed before reviewing makes sense again labels May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SNMP Monitor #4717

feat: Add SNMP Monitor #4717

mattv8 commented Apr 27, 2024 •

edited by CommanderStorm

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattv8 commented Apr 30, 2024 •

edited

CommanderStorm left a comment

CommanderStorm Apr 30, 2024 •

edited

mattv8 May 2, 2024 •

edited

CommanderStorm commented May 23, 2024

chakflying commented May 24, 2024

chakflying commented May 24, 2024

mattv8 commented May 28, 2024 •

edited

feat: Add SNMP Monitor #4717

Are you sure you want to change the base?

feat: Add SNMP Monitor #4717

Conversation

mattv8 commented Apr 27, 2024 • edited by CommanderStorm

Description

Type of change

Checklist

Screenshots

This comment has been minimized.

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mattv8 commented Apr 30, 2024 • edited

CommanderStorm left a comment

Choose a reason for hiding this comment

CommanderStorm Apr 30, 2024 • edited

Choose a reason for hiding this comment

mattv8 May 2, 2024 • edited

Choose a reason for hiding this comment

CommanderStorm commented May 23, 2024

chakflying commented May 24, 2024

chakflying commented May 24, 2024

mattv8 commented May 28, 2024 • edited

mattv8 commented Apr 27, 2024 •

edited by CommanderStorm

mattv8 commented Apr 30, 2024 •

edited

CommanderStorm Apr 30, 2024 •

edited

mattv8 May 2, 2024 •

edited

mattv8 commented May 28, 2024 •

edited